Show canonical ACL overlap remediation as a green status line, not a yellow warning - #45
Merged
Joel Platek (VAsHachiRoku) merged 2 commits intoAug 24, 2026
Conversation
…yellow warning When OU disable-inheritance remediation re-sorts a DACL that contains an explicit Deny and Allow for the same principal (e.g. Exchange Trusted Subsystem), Repair-TierModelCanonicalAcl previously emitted a long yellow WARNING per overlapping pair per OU ? a wall of alarming text during an expected, successful operation. Replace that with a single green "REMEDIATED: Canonical ACL overlap on OU: <name>" line per OU (deduplicated). The detailed per-principal entry (DN + principal) is still recorded in the result's Warnings array for logging and audit. Visual/UX change only ? no change to the sort or repair logic. Two overlap tests updated to assert on the result's Warnings array instead of the warning stream. Full suite 1627/0.
Copilot started reviewing on behalf of
Joel Platek (VAsHachiRoku)
August 24, 2026 06:46
View session
There was a problem hiding this comment.
Pull request overview
This PR adjusts the operator-facing output for canonical ACL overlap remediation during OU deployment: instead of emitting repeated yellow WARNING: lines on the warning stream, it now prints a single green remediation status line per OU while still recording detailed overlap entries in the returned result’s Warnings array.
Changes:
- Updated
Repair-TierModelCanonicalAclto deduplicate terminal output for Deny/Allow overlap scenarios and to store overlap details inWarningswithout writing to the warning stream. - Updated overlap-related unit tests to assert against the result’s
Warningsarray instead of-WarningVariable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| modules/TierModel/public/Repair-TierModelCanonicalAcl.ps1 | Changes overlap reporting from warning-stream output to a single green status line + structured Warnings entries. |
| tests/Unit.CanonicalAclRepair.Tests.ps1 | Updates overlap tests to validate the Warnings array rather than capturing warning-stream output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+198
to
+202
| if (-not $printedOverlapNotice) { | ||
| $ouLeaf = if ([string]::IsNullOrEmpty($DistinguishedName)) { $DistinguishedName } else { (($DistinguishedName -split ',')[0] -replace '^\w+=','') } | ||
| Write-Host "REMEDIATED: Canonical ACL overlap on OU: $ouLeaf" -ForegroundColor Green | ||
| $printedOverlapNotice = $true | ||
| } |
Copilot started reviewing on behalf of
Joel Platek (VAsHachiRoku)
August 24, 2026 07:25
View session
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
tests/Unit.CanonicalAclRepair.Tests.ps1:334
- In this overlap context, the last two tests still pass
-WarningVariable warnVar 3>$null, even though overlap notices are no longer emitted on the warning stream. Keeping the redirection here can mask unexpected warning-stream output from future changes and makes the tests inconsistent with the new contract (Warnings array on the result).
(($result.Warnings) -join ' ') | Should -Match 'overlap|S-1-5-11|Authenticated'
}
It "Still repairs (IsCanonical=true) despite the overlap warning" {
$result = Repair-TierModelCanonicalAcl -SecurityDescriptorBytes $script:OverlapInputBytes -WarningVariable warnVar 3>$null
Comment on lines
+199
to
+200
| $ouLeaf = if ([string]::IsNullOrEmpty($DistinguishedName)) { $DistinguishedName } else { (($DistinguishedName -split ',')[0] -replace '^\w+=','') } | ||
| Write-Host "REMEDIATED: Canonical ACL overlap on OU: $ouLeaf" -ForegroundColor Green |
Joel Platek (VAsHachiRoku)
deleted the
fix/canonical-remediation-message
branch
August 24, 2026 07:31
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improves the terminal output during OU deployment: the canonical-ACL overlap notice is now a concise green status line instead of a long yellow WARNING.
Background
When
New-TierModelOudisables inheritance on a Tier OU and re-sorts the DACL into canonical order,Repair-TierModelCanonicalAclchecks for an explicit Deny and Allow for the same principal with overlapping rights (a legitimate, common pattern — e.g.Exchange Trusted Subsystem). Previously it emitted a yellowWARNING:line per overlapping pair, per OU. On a domain with an inherited Deny, a successful deployment of ~7 OUs produced a wall of alarming yellow text — even though the remediation worked correctly and is expected behavior.Change
REMEDIATED: Canonical ACL overlap on OU: <name>(deduplicated, one per OU).Warningsarray for logging/audit — only the terminal presentation changed.Tests
Two overlap tests in
tests/Unit.CanonicalAclRepair.Tests.ps1updated to assert on the result'sWarningsarray (the notice is no longer on the warning stream). Full suite: 1,627 passing, 0 failing; coverage unchanged (~91% on the module scope).Notes
Visual/UX only — no functional or version-affecting change. Independent of the
.gitattributesline-ending fix (separate branch/PR).